Skip to content
This repository was archived by the owner on Sep 15, 2025. It is now read-only.

Conversation

@crazytonyli
Copy link
Contributor

Description

There are some difficulties in using a binary package. This PR refactors the source code a little bit so that we can provide source code instead of a pre-built binary.

I have split the changes into individual self-contained commits. Most of the diff comes from adding import statements. The actual code changes are contained in their own commit, which only includes a small number of changes.

Next steps

We don't need to merge this PR. Once this PR passes code review, I'll copy the source code over to the WordPress-iOS repository. And, the app will stop using this library at that point.

After that, I'll copy the test code over to the WordPress-iOS project and set up a new test target there, so that the existing WordPressKit unit tests continue to run as part of the app CI pipeline.

Testing Details

ℹ Please replace this with a clear and concise description of the steps required to validate this pull request.


  • Please check here if your pull request includes additional test coverage.
  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

@crazytonyli crazytonyli requested a review from kean August 28, 2025 00:23
@dangermattic
Copy link
Collaborator

2 Warnings
⚠️ Package.swift was changed without updating its corresponding Package.resolved. Please resolve the Swift packages as appropriate to your project setup (e.g. in Xcode or by running swift package resolve).
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

public var avatarURL: String?
public var dateCreated: Date?
public var emailVerified: Bool = false
public var linkedUserID: NSNumber?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original properties in the original Objective-C code are null_unspecified or implicitly unwrapped optional. I can keep the original code and make the new properties IUO, too. But that does not feel right.

The new code now uses optional, which means the app will need to update their codebase to deal with the new optional properties instead of IUO properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to translate this function to Swift, because it's used in a Swift module. I'll explain a little bit more of why this is needed in the upcoming app PR (which would provide a full context).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. We'll just need to make sure we test the features that needed rewrite.

extension ServiceRemoteWordPressComREST {
public var wordPressComRestApi: WordPressComRestApi {
self.wordPressComRESTAPI as! WordPressComRestApi
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to use an interface type in the ServiceRemoteWordPressComREST.h, because the implementation is in the Swift WordPressKit module.

In practice, WordPressComRestApi is the only acceptable instance to create ServiceRemoteWordPressComREST, so, we'll do a little bit hack here to force cast in runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the WordPressOrgXMLRPCApi property below.

Copy link
Contributor

@kean kean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! And it's a straightforward changes with minimum code conversions.

@crazytonyli
Copy link
Contributor Author

As mentioned in the PR description, I'll close this PR as the changes are being integrated into the app repo.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants